-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Scaffolding Standard Proposal #33
Conversation
src/config/api/api.types.ts
Outdated
} | ||
|
||
export interface ServiceResponse<T> { | ||
data: T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest changing this to payload
instead of data
because data
is already used by the axios instance. This way in our api calls instead of having response.data.data
we would have response.data.payload
. Word doesn't have to be payload
, that's what makes sense to me.
I've seen at least three or four times confusion when trying to access the data in a response and people not realizing they have to access data twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I agree that data
isn't the best because it's weird having response.data.data
, this interface represents the structure the https://github.com/flugg/laravel-responder package provides (which we use for the responses). We should make a poll if we want to stop using the data
key because it would involve updating the backend boilerplate so that it changes this key to what we decide.
I'm afraid I have to disagree with the use of payload
because it refers to data sent by the client to the server, not something you get from the server back to the client. We would have to think of something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about this guys?
cc: @JLin-99 @guillermo-lightit @StokJoaco @cesarlightit @gianfranco-rocco @thomasmcculloch2 @ezef @cgarcia-lightit @UrielRadzyminski1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree that payload is more semantic and avoids redundancy in names 👍
Data doesn't explain the context. Payload does.
But I can live with data if backend team refuses to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @gianfranco-rocco that the naming payload
might not be the best choice, as he mentioned, it's a term typically used when the client sends data to the server and no the other way. So I would prefer to keep with data
What I find strange here (and I think that's where the problem lies) is that the data
field contains data from the response
, when in reality it should only contain the data
itself.
but we are duplicating and getting those response information inside data
... that's why we're using response.data.data
instead of response.data
directly or response.data.pagination
instead of response.pagination
Ideally, the data
field should only contain the actual data
itself, without any additional response information.
Example from other app's GET response:
Example from our app's GET response:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I see it we have three choices here:
- Write a custom serializer for Laravel Responder that replaces the
data
key for something else. - Write a custom interceptor for Axios that replaces the
data
key for something else. - Do nothing and keep
response.data.data
as our standard.
My preferred option would be number one but I'm fine with all three of them to be honest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on not using payload
as the nomenclature for a service response, since its typically used for, well, body payloads (requests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I for example usually refer to https://jsonapi.org/format/#document-structure or (a simpler) https://github.com/omniti-labs/jsend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion I prefer option 2 or 3 from Walter list, but in case we do the interceptor, I'm not sure how we should name the data (+1 not using payload). So in general, I'm ok with data.data tbh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we decide to change the standard, the 1st option is the best IMO.
src/config/api/api.types.ts
Outdated
|
||
export interface ServiceResponse<T> { | ||
data: T; | ||
pagination: ServicePagination; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are planning to use ServiceResponse
also for error responses maybe pagination
should be optional? Even for the success case, I can see scenarios where we don't want to paginate the results.
In case we also use these interface for errors we should talk about what would the T
type looks like in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The success and error responses are slightly different, so it would make sense to have separate interfaces.
Successful response:
{
"status": 200,
"success": true,
"data": null
}
Error response:
{
"success": false,
"status": 500,
"error": {
"code": null,
"message": null
}
}
Also, pagination
is only present in successful responses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, it would be best to have three interfaces. One for success responses without pagination, one for success responses with pagination, and one for error responses.
export interface SuccesServiceResponse<T> {
status: number;
success: true;
data: T;
}
export interface PaginatedServiceResponse<T> extends SuccesServiceResponse<T> {
pagination: PaginationData;
}
export interface ErrorServiceResponse {
status: number;
success: false;
error: {
code: string|null;
message: string|null;
};
}
I believe having a separate interface for paginated responses is better because:
- You have an easy way of knowing which responses are paginated and which are not just be looking at the code
- You remove the need of having to check if pagination is present and has a value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gianfranco-rocco @ThunderNaka It would be great for ServiceResponse to be an union of both success and error response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gianfranco-rocco @ThunderNaka It would be great for ServiceResponse to be an union of both success and error response
Huh? How so?
user: User | null; | ||
setUser(user: User | null): void; | ||
token: string | null; | ||
setToken(token: string | null): void; | ||
} | ||
|
||
export const useUserStore = create<UserStoreState>()( | ||
export const useAuthStore = create<AuthStoreState>()( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to propose a different approach for structuring stores where we separate setters like setUser
or setToken
from the actual state user
and token
as outlined in this article from the zustand docs: https://zustand.docs.pmnd.rs/guides/practice-with-no-store-actions
Besides the small benefits described in the article, I've also found that when persisting the store to localStorage
these function definitions do not serve any purpose and can also cause issues being not serializable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think using this as a standard but also gathering everything from this file in a namespace in the barrel file for example:
export * as authStore from "./useAuthStore";
This approach allows us to achieve both objectives: separating concerns while avoiding multiple setters with similar names, as often seen in regular component states or sections.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sounds good!
We should think about introducing the concept of "services". By Some examples of these systems would be:
The reason I bring this up is that with the current implementation some of these
To me, all of these should be treated in the same way regarding app architecture, so some options could be:
Let me know your thoughts on this. |
What do you think about this guys? |
Services sounds goods to me 👍 |
src/config/providers/index.ts
Outdated
export * from "./Providers"; | ||
export * from "./ReactQueryProvider"; | ||
export * from "./SentryProvider"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we only export Providers
? I thought the idea was for the providers
folder to encapsulate all the providers, and to only export Providers
src/config/api/api.types.ts
Outdated
pageSize?: number; | ||
sort?: string; | ||
filter?: T; | ||
with?: string | string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to promote using with
for loading relationships from the frontend? I know it's a feature that exists in our backends, but I remember that at some point we were discussing this is a bad practice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How else would you load relationships instead if you don't promote using the feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You just don't load them from the frontend, you define the relationships that are returned by an endpoint in the backend, and if you need different versions you can create different endpoints.
It's a matter of who's responsible for defining the response of an endpoint.
export const RouterWrapper = ({ guest = false, children }: Props) => { | ||
const isAuthenticated = useAuthStore((state) => state.token); | ||
|
||
if (!guest && !isAuthenticated) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between guest
and isAuthenticated
? Why do we need both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgoycoechea-lightit Guest is for routes that are available for anyone e.g. Login, Register, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can change the prop name to "Public" or something more clear, because "guest" suggests is a user property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about not showing those routes to authenticated users ?
Why should an authenticated user visit the register page?
This for example is an extract of a project:
// routes.tsx
{
element: <AuthenticatedRoute roles={[ROLES.admin]} />,
children: [
{
path: PATHS.externalApis,
element: <ExternalApiSettingsScreen />,
},
{
path: `${PATHS.externalApis}/:id`,
element: <ExternalApiDetailsScreen />,
},
{
path: PATHS.reports,
element: <ReportsScreen />,
},
],
},
{
element: <UnauthenticatedRoute />,
children: [
{
path: '/',
element: <Navigate to={PATHS.account.login} replace />,
},
{
path: PATHS.account.register,
element: <RegisterScreen />,
},
{
path: PATHS.account.login,
element: <LoginScreen />,
},
{
path: PATHS.account.forgotPassword,
element: <ForgotPasswordScreen />,
},
],
},
{ path: '*', element: <NotFoundScreen /> },
// UnauthenticatedRoute
export const UnauthenticatedRoute = () => {
const user = useAuthStore((state) => state.user);
if (user) return <Navigate to={PATHS.home} replace />;
return <Outlet />;
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the "Public" Idea from Ferna, I see some cases in where that can be useful, maybe not for a register, but maybe for some info pages or things like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support the idea of having private / public routes.
src/stores/useAuthStore.ts
Outdated
@@ -26,7 +26,7 @@ export const useUserStore = create<UserStoreState>()( | |||
}, | |||
}), | |||
{ | |||
name: "feedbackUserStore", | |||
name: "feedbackAuthStore", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd name it AuthStore
or useAuthStore
name: "feedbackAuthStore", | |
name: "AuthStore", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the ui
folder is inside components
now? I think this is a bit confusing.
I like having 3 levels for defining components (ui
, components
, and sections
), but I find it weird that we have components
and sections
at the same level and ui
inside components
.
src/config/api/api.types.ts
Outdated
} | ||
|
||
export interface ServiceResponse<T> { | ||
data: T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm backing up @gianfranco-rocco here.
src/config/api/api.types.ts
Outdated
|
||
export interface ServiceResponse<T> { | ||
data: T; | ||
pagination: ServicePagination; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gianfranco-rocco @ThunderNaka It would be great for ServiceResponse to be an union of both success and error response
export const RouterWrapper = ({ guest = false, children }: Props) => { | ||
const isAuthenticated = useAuthStore((state) => state.token); | ||
|
||
if (!guest && !isAuthenticated) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgoycoechea-lightit Guest is for routes that are available for anyone e.g. Login, Register, etc
export const RouterWrapper = ({ guest = false, children }: Props) => { | ||
const isAuthenticated = useAuthStore((state) => state.token); | ||
|
||
if (!guest && !isAuthenticated) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can change the prop name to "Public" or something more clear, because "guest" suggests is a user property
src/stores/useAuthStore.ts
Outdated
@@ -26,7 +26,7 @@ export const useUserStore = create<UserStoreState>()( | |||
}, | |||
}), | |||
{ | |||
name: "feedbackUserStore", | |||
name: "feedbackAuthStore", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not persist authStore in session storage?
const useCreate{{pascalCase name}} = useMutation({ | ||
mutationFn: create{{pascalCase name}}, | ||
onSuccess: (new{{pascalCase name}}) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also type the return type of the mutation by default
export const use{{pascalCase name}} = () => { | ||
const queryClient = useQueryClient(); | ||
|
||
const use{{pascalCase name}}List = (params: Base{{pascalCase name}}ListParams) => | ||
useQuery({ | ||
...{{camelCase name}}Keys.list, | ||
queryFn: () => get{{pascalCase name}}List(params), | ||
}); | ||
|
||
const use{{pascalCase name}}Detail = ({{camelCase name}}Id: string) => | ||
useQuery({{camelCase name}}Keys.detail({{camelCase name}}Id)); | ||
|
||
const usePrefetch{{pascalCase name}}Detail = ({{camelCase name}}Id: string) => | ||
queryClient.prefetchQuery({{camelCase name}}Keys.detail({{camelCase name}}Id)); | ||
|
||
const useCreate{{pascalCase name}} = useMutation({ | ||
mutationFn: create{{pascalCase name}}, | ||
onSuccess: (new{{pascalCase name}}) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed my mind with this approach, with @joako-gaona and @walt-it, we thought that it would be best for the "parent" hook to not exists, but all hooks to be individual modules in the same file. For calling the queryClient you could just make the declaration inside the hook being called. e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way you would have cleaner hook calls in the components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've pushed the new idea.
const useCreate{{pascalCase name}} = useMutation({ | ||
mutationFn: create{{pascalCase name}}, | ||
onSuccess: (new{{pascalCase name}}) => { | ||
queryClient.setQueryData( | ||
{{camelCase name}}Keys.detail(new{{pascalCase name}}.id).queryKey, | ||
new{{pascalCase name}}, | ||
); | ||
|
||
void queryClient.invalidateQueries({ | ||
queryKey: {{camelCase name}}Keys._def, | ||
}); | ||
}, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const useCreate{{pascalCase name}} = useMutation({ | |
mutationFn: create{{pascalCase name}}, | |
onSuccess: (new{{pascalCase name}}) => { | |
queryClient.setQueryData( | |
{{camelCase name}}Keys.detail(new{{pascalCase name}}.id).queryKey, | |
new{{pascalCase name}}, | |
); | |
void queryClient.invalidateQueries({ | |
queryKey: {{camelCase name}}Keys._def, | |
}); | |
}, | |
}); | |
const useCreate{{pascalCase name}} = () => { | |
const queryClient = useQueryClient(); | |
return useMutation({ | |
mutationFn: create{{pascalCase name}}, | |
onSuccess: (new{{pascalCase name}}) => { | |
queryClient.setQueryData( | |
{{camelCase name}}Keys.detail(new{{pascalCase name}}.id).queryKey, | |
new{{pascalCase name}}, | |
); | |
void queryClient.invalidateQueries({ | |
queryKey: {{camelCase name}}Keys._def, | |
}); | |
}, | |
}); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this
path: "src/domains/{{kebabCase name}}/queries/useSomethingQueryExample.tsx", | ||
path: "src/domains/{{kebabCase name}}/queries/{{kebabCase name}}.tsx", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If going to implement the change of the hooks being defined modular instead of in a big hoook this name is fine, else I would like even more a naming like "use{{pascalCase name}}.tsx
In which case something in the ui folder will not be a component? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand ok, the idea is to have all queries in one single file?
I think of components like bigger units than what we currently have in |
I see your point. Maybe we can apply the principles of the Atomic Design Pattern, without necessarily using the same terminology. |
Co-authored-by: Hernán Yáñez <[email protected]>
Co-authored-by: Hernán Yáñez <[email protected]>
5bb639b
to
1dbb752
Compare
}; | ||
|
||
export const RouterWrapper = ({ guest = false, children }: Props) => { | ||
const isAuthenticated = useAuthStore((state) => state.token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how are we going to handle this if we are using HTTP only cookies?
Scaffolding Standard Proposal
This proposal defines a simple, scalable folder structure for our project.
The aim is to ensure ease of use and collaboration by promoting consistency and accessibility.
By organizing files modularly—both within specific domains and across shared resources—we enhance maintainability and foster better team collaboration.
Top-Level Structure
Config
API:
In the
api
folder, we will centralize the main API configuration and its interceptors. This includes setting up the base URL, configuring headers, and handling request/response interception (e.g., for adding tokens or handling errors globally). This folder should be the single management point for all API interactions, ensuring consistency and ease of updates across the application.Constants:
This folder will contain constants tightly coupled to this project and specific to its logic. These constants should not be reused across other repositories. If you find that the constants could be shared with other projects, it might be better to move them to other folder to promote reusability and cleaner code separation.
Providers:
We’ve set up two Providers and a combined Provider that aggregates both. The main purpose here is to centralize third-party library configurations—such as
react-query
for data fetching orSentry
for error monitoring. This helps keep the entry point clean and focused on application-specific logic, promoting a better separation of concerns. Other configurations, like Stripe or analytics tools, could also be housed here. The idea is to have a designated space where all external library integrations are managed consistently, making the architecture modular and easy to maintain.Scripts:
The scripts folder is dedicated to housing all the scripts that need to run at the application's entry point. These scripts typically handle initialization tasks, environment-specific setups, and external service integrations, allowing for a clean and maintainable structure.
Some key functions of this folder include:
Domains
The main folder where we define domains using the command
pnpm gen:domain
; each subfolder will be a domain, and we'll have a barrel file exporting every domain. What's a domain? A set of screens where we share components, queries, and API calls, we define the boundary. From each domain, we do not export any local components or sections. We will only export the domain router/screen.domain-a
is an example.API:
The
api
folder within each domain contains all API-related logic specific to that domain. This includes API calls, request handlers, and utility functions related to data fetching or manipulation for the domain's features. Keeping API logic domain-specific avoids global dependencies and makes updating or managing changes related to a particular feature easier.Component:
The
components
folder houses UI components specific to the domain. These components are not exported outside the domain but are shared across different screens within the domain. This ensures modularity and prevents unnecessary coupling between domains, fostering a well-organized component hierarchy.Context:
The
context
folder defines context providers and consumers that manage the domain-specific state. Context is used when state needs to be shared between different parts of the domain, such as within screens and components. Keeping context local to the domain ensures that the state management logic is isolated and not inadvertently shared across unrelated domains.Queries:
The
queries
folder holds all data-fetching logic using libraries like react-query or similar tools. These queries handle retrieving data related to the domain, managing caching, error states, and optimizations. Centralizing queries within a domain reduces redundancy and keeps the domain self-contained.Screens:
The
screens
folder contains the main views or pages of the domain. Each screen represents a full user interface, such as a dashboard or detail view. Screens typically utilize components, context, and queries from the domain to render data and handle user interactions.Sections:
The
sections
folder organizes sub-sections of screens that can be seen as larger components or layout blocks. These are not exported outside the domain but can be reused across multiple screens within the same domain. Sections are useful for grouping related content or functionalities within a screen, maintaining a clear structure while keeping the code modular.We ensure a scalable and modular architecture by organizing the project into domains with these well-defined subfolders. Each domain encapsulates its functionality, reducing dependencies between different application parts.
The Frontend team of each project can define which folders should be kept or left out.
Router
Route colocation will be here. Anything that has to do with routes. This is a discussion point because I'm not sure yet about this solution.
Shared
The
shared
folder serves as a centralized hub for reusable resources across different domains. It is designed to be modular so that it can be easily copied into other repositories and function seamlessly. Placing shared logic, UI elements, and utilities in this folder eliminates duplication and promotes consistency throughout the application. This structure enhances maintainability, as updates to shared elements only need to be made in one location, with changes automatically propagated across all domains that rely on them.Assets:
The
assets
folder contains static files like images, fonts, and other media used across domains. Keeping assets in this shared location ensures that they can be easily referenced throughout the application without duplicating files. It also allows for a more organized way to manage and optimize assets globally.Components:
We gather reusable UI components used across multiple domains in the
components
folder. These components are generic enough to be applied in various contexts, such as buttons and form inputs, to name a few. By abstracting these into the shared folder, we ensure that the same styles and behaviors are applied consistently throughout the application, reducing redundancy and simplifying updates.Hooks:
The
hooks
folder holds custom React hooks that manage shared logic or side effects, state management, or common event handlers. These hooks are built to be reusable across different domains and ensure that shared logic can be abstracted away from the domain-specific code, promoting clean and DRY (Don't Repeat Yourself) principles.Sections:
In the
sections
folder, we define reusable larger UI blocks that can be composed into screens across different domains. Sections could include header layouts, footers, or modular page structures shared across various app parts. Like components, these sections provide a consistent look and feel throughout the application.Utils:
The
utils
folder contains utility functions that are shared across domains. These functions typically handle common tasks such as data formatting, validation, or mathematical calculations. By centralizing utility logic in the utils folder, we reduce the risk of code duplication and provide a single source of truth for these helper functions across the project.By leveraging the
shared
folder, the application becomes more modular, allowing for easy reuse and reducing the maintenance burden when updates or changes are needed.Stores:
Here, we locate every zustand store. Usually, we should have access from anywhere when we define a store. Nonetheless, you can also have a store folder in each domain, but it's not intended to be this way.
To Discuss
Detailed List
Comment for reviewers
Go hard with comments on this PR. Every detail.